Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] WIP: Initial Action and StatusEffect implementation #958

Merged
merged 56 commits into from
Jun 21, 2024

Conversation

arieshi255
Copy link
Contributor

Still a WIP, but can be useful for some other things, so submitting this now.
Development will be done on want_some_action.

arieshi255 and others added 30 commits March 6, 2023 23:40
[3.x] Some initial action work (mostly statuses)
[3.x] More action work, begin implementing jobs
{
case ClassJob::Warrior:
{
Warrior::onAction( *m_pSource->getAsPlayer(), *this );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm okay with this as is for now, but i think it would probably better to have an array or otherwise that's just indexed by classjob (which also lets you pick up the base classjob impl easy) instead of this - i get this is wip-ish though so i'm not too worried about it at the moment

@NotAdam NotAdam merged commit 7bfd953 into SapphireServer:master Jun 21, 2024
4 checks passed
@@ -430,7 +430,7 @@ namespace Excel
uint8_t HideCastBar : 1;
uint8_t IsTargetLine : 1;

int8_t padding0;
int8_t unknown : 8;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why : 8 ? it is already an 8 bit value.

actor.removeSingleStatusEffectById( WrathII );
actor.removeSingleStatusEffectById( WrathIII );
actor.removeSingleStatusEffectById( WrathIV );
actor.removeSingleStatusEffectById( Infuriated );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be better to have something that can remove a list of ids if that is required.

@@ -40,6 +40,8 @@ struct StatusModifier
struct StatusEntry
{
uint16_t id;
int32_t duration;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the duration be negative? If not, unsigned makes more sense.

@@ -583,13 +590,72 @@ void Action::Action::buildActionResults()
shouldRestoreMP = false;
}
}

// If we hit an enemy
if( m_hitActors.size() > 0 && getHitChara()->getObjKind() != m_pSource->getObjKind() )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd prefer !m_hitActors.empty(), it is more elaborate


handleJobAction();

if( m_lutEntry.statuses.caster.size() > 0 || m_lutEntry.statuses.target.size() > 0 )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here. !empty() is cleaner than size() > 0

@@ -89,6 +101,19 @@ void ActionResult::applyStatusEffectSelf( uint32_t id, int32_t duration, uint8_t
m_pStatus->setParam( param );
}

void ActionResult::applyStatusEffectSelf( uint32_t id, int32_t duration, uint8_t param, std::vector< World::Action::StatusModifier > modifiers,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned, i think it would be good to not pass a copy of the vector here.

break;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a map, why do you iterate it to find an id? You should be able to jund "find()" it.

{
if( effectIt.second->getId() == id )
return effectIt.second;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here, of there is no proper indexing for it, it might be worth to create such a lookup. iterating the entire list all the time might be overkill.

return m_flag;
}

std::vector< World::Action::StatusModifier > Sapphire::StatusEffect::StatusEffect::getStatusModifiers() const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably return a const ref

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants